[Variant] Add variant_get access as Variant - #9681
Conversation
|
@scovich Please take a look when available! |
Co-authored-by: Ryan Johnson <scovich@users.noreply.github.com>
|
Looks like there's a merge conflict? |
|
@scovich fixed |
|
@scovich could you take a look again? |
| @@ -204,7 +204,23 @@ fn shredded_get_path( | |||
| // Helper that shreds a VariantArray to a specific type. | |||
| let shred_basic_variant = | |||
| |target: VariantArray, path: VariantPath<'_>, as_field: Option<&Field>| { | |||
There was a problem hiding this comment.
This helper just became a lot more complicated, could use some inline code comments explaining how this new requested_variant stuff impacts the control flow?
e.g. "if the caller requested variant, unshred to binary it before continuing; if they also provided a path to traverse, don't pass a type so that the requested field can be fetched directly."
But that opens a bunch of questions:
- Why unshred the whole thing if the user provided a non-empty
path? Wouldn't it be better to any shredding as deeply as possible first? Or did the caller ofshred_basic_variantalready do that? - This code seems to ignore any shredded variant schema the user might have provided. I get that we don't necessarily want to tackle variant-get with custom shredding in this PR, but we should at least block it with an error instead of silently returning something different than was requested?
There was a problem hiding this comment.
- It does already do that.
- Hmm... It's [Variant] Support Shredded Objects in
variant_get: access asSome(DataType::Struct)(nested shredding) #8153.
I could do 2 but I'm not sure about variant_get custom shredding.
I checked Spark/duckdb/dremio variant_get implementations and it seems like no one does output shredding as a part of variant_get which is basically:
variant_get(arr, path, Some(shredding_schema)) ≡ shred_variant(variant_get(arr, path), schema)There's a https://github.com/datafusion-contrib/datafusion-variant Variant integration and it strives for parity with spark/databricks.
Custom shredding will, however, skip unshred-reshred round trip that'd come from doing:
shred_variant(variant_get(arr, path), schema)That's really a #8153 discussion
There was a problem hiding this comment.
Fair! So we should just check for and block typed_value in the variant schema, to avoid any confusion.
There was a problem hiding this comment.
Done here cf7e545. Sorry, should've split the comments/change/nit into separate commits
Co-authored-by: Ryan Johnson <scovich@users.noreply.github.com>
|
@scovich PTAL again. |
|
|
|
@alamb PTAL 😎 |
|
I'll merge it on @scovich 's approval :) Any chance you can resolve the merge conflicts? |
|
gogogo thanks again |
Which issue does this PR close?
variant_get: Access asVARIANT(Unshredding) #8154.Rationale for this change
Check issue
What changes are included in this PR?
variant_getaccess asVariantAre these changes tested?
Yes, added unit tests
Are there any user-facing changes?
Can now extract unshredded
VariantArraywithvariant_get